Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(templates): disable use of jsonnet with /api/v2/templates/apply #23030

Merged
merged 1 commit into from
Dec 30, 2021

Conversation

williamhbaker
Copy link
Contributor

@williamhbaker williamhbaker commented Dec 29, 2021

Disables server-side jsonnet processing to eliminate potential issues with the go-jsonnet implementation.

@williamhbaker williamhbaker changed the title fix(templates): disable use of jsonnet with /api/v2/templates/apply fix(templates): disable use of jsonnet with /api/v2/templates/apply Dec 29, 2021
@williamhbaker williamhbaker marked this pull request as ready for review December 29, 2021 17:01
@williamhbaker williamhbaker requested a review from a team as a code owner December 29, 2021 17:01
Copy link
Contributor

@davidby-influx davidby-influx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking question about error information (current code may be correct for security reasons), but if not a security issue, error handing could be more informative.


for _, rem := range remotes {
// While things like '.%6Aonnet' evaluate to the default encoding (yaml), let's unescape and catch those too
decoded, err := url.QueryUnescape(rem)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to return the error here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or does that let too much security information escape?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may be some security concern, although there shouldn't be much of an issue since our api.Err(...) methods return sanitized errors to clients. Mostly I am keeping these consistent with the IDPE error messages.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I see @jdstrand has already merged the code in IDPE, so probably we shouldn't become incompatible.

Copy link
Contributor

@jdstrand jdstrand Jan 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're referring to the specific error from url.QueryUnescape() instead of a generic error, I erred on the side of caution since I felt legitimate use of the API should not be causing this error and so giving the attacker a generic error was fine. If we find this to be problematic in practice, we can return a more specific error.

pkger/service.go Show resolved Hide resolved
pkger/service.go Show resolved Hide resolved
@williamhbaker williamhbaker merged commit 11c0081 into master Dec 30, 2021
@williamhbaker williamhbaker deleted the wb-templates-apply-jsonnet-fix branch December 30, 2021 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants